fix(pg-connection-string): get closer to libpq semantics for `sslmode` #2709

pmalouin · 2022-02-21T22:21:54Z

Summary

We have found that the handling of the sslmode connection string parameter is inconsistent with other PG libraries and with the reference libpq documentation. This PR proposes some changes to sslmode behavior that are more aligned with libpq.

Detailed `sslmode` behavior

Here is the list of all sslmode values and their expected behavior with this PR:

`sslmode=verify-full`

Require an SSL connection and verify the CA and server identity.
No changes in this PR.

`sslmode=verify-ca` (changed)

Require an SSL connection and verify the CA, but not the server identity. This is achieved by setting ssl.checkServerIdentity to a no-op function (see docs). Previously, this mode behaved like verify-full but that was not consistent with the libpq implementation.

`sslmode=require` (changed)

If a root CA is provided via the sslrootcert parameter of the connection string, it behaves like verify-ca. Otherwise, require an SSL connection but do not verify CA and server identity (ssl.rejectUnauthorized is set to false).
Previously, this mode behaved like verify-full but that was not consistent with the libpq implementation.

`sslmode=no-verify`

Require an SSL connection but do not verify CA and server identity (ssl.rejectUnauthorized is set to false).
No changes in this PR. Note: this mode is not documented in libpq and does not appear to be broadly supported in other libraries, but has been kept as-is for the sake of backwards-compatibility. One option could be to mark it as deprecated since sslmode=require could be an alternative, but doing so might have little value for this project.

`sslmode=prefer` (changed)

Require an SSL connection but do not verify CA and server identity (ssl.rejectUnauthorized is set to false). Previously, this mode behaved like verify-full but that was not consistent with the libpq implementation.
In reality, this mode should be even less strict and support a fallback logic from SSL to non-SSL connection if SSL is not accepted by the server. Implementing a fallback logic seems to be more complex to solve and I did not dare touch this, but this could eventually be addressed if users of this library deem this mode valuable.

`sslmode=allow`

Not supported by this library.
No changes in this PR. For this mode also, there could be an opportunity to implement a fallback logic from non-SSL to SSL, but I did not dare touch this and I don't have data that suggests that this might be valuable for this project.

`sslmode=disable`

Only try a non-SSL connection.
No changes in this PR

An important note is that this PR potentially introduces semver breaking changes, in particular because it relaxes the security constraints of some sslmode values:

sslmode=prefer is less strict, users should switch to sslmode=verify-full to keep parity.
sslmode=require is less strict, users should switch to sslmode=verify-full to keep parity.
sslmode=verify-ca is less strict, users should switch to sslmode=verify-full to keep parity.

Prior discussions about `sslmode`

I believe this PR addresses concerns raised in these two GH issues in the past:
#2281
#2009

In particular, there has been one case where the sslmode=verify-ca is currently too strict when connecting through AWS RDS Proxy, but the work-around of using sslmode=no-verify would disable CA verification completely.

Other languages/libraries and their support for `sslmode`

Just as a reference, these two libraries are also trying to be more or less consistent with libpq:

Golang - pq
- Supports disable, require, verify-ca andverify-full only.
PostgreSQL JDBC Driver
- Supports all 6 modes.

Thanks for considering this change and please let me know how I can polish this further for acceptance 🙏

packages/pg-connection-string/README.md

packages/pg-connection-string/test/parse.js

cyx · 2022-03-24T00:18:45Z

@charmander do we have an idea when this will land / get released as part of pg9?

ceefour · 2022-12-28T05:17:51Z

Please merge this as it fixes #2281 and #2375

ceefour · 2023-03-16T04:54:29Z

Help @brianc to merge this

ThisIsMissEm · 2024-01-22T21:45:52Z

@brianc / @hjr3 what's the status on this? We've a PR on Mastodon blocked by this change: mastodon/mastodon#25483

charmander · 2024-01-23T07:16:56Z

@ThisIsMissEm Is that PR blocked on this? It looks more like it would be made unnecessary by this.

ThisIsMissEm · 2024-01-23T07:48:33Z

@ThisIsMissEm Is that PR blocked on this? It looks more like it would be made unnecessary by this.

I'm saying "blocked" in the sense of if upstream is going to fix, that'd be preferable to a fix in our code for it

charmander · 2024-01-23T07:58:05Z

@ThisIsMissEm On second read, it looks like the Mastodon PR is necessary either way. Unless you were planning on deleting all of that code if this were merged and released soon enough?

hjr3 · 2024-01-23T11:24:02Z

This PR introduces breaking changes. We should probably:

merge feat(pg-connection-string): ClientConfig helper functions #3128
release version 2.8.0
merge this PR
see if there are any other breaking changes that need to be made
release version 3.0

ThisIsMissEm · 2024-01-23T12:44:29Z

This PR introduces breaking changes. We should probably:

merge feat(pg-connection-string): ClientConfig helper functions #3128

release version 2.7.0

merge this PR

see if there are any other breaking changes that need to be made

release version 3.0

@hjr3 that sounds good to me!

ThisIsMissEm · 2024-08-10T14:58:58Z

Just wanted to follow up and see if there's any plans for forwards progress on this?

Trott · 2024-11-12T22:38:33Z

Over on Mastodon, @ThisIsMissEm asked if I might know some Node.js TLS/SSL people who might be able to review this. I hope I'm not pinging too widely, but I'm certainly interested in trying to unblock things for the Mastodon project so....hey, can anyone review? @indutny @jasnell @joyeecheung @tniessen @pimterry @panva @mertcanaltin

charmander · 2024-11-13T01:14:15Z

I’m not really up to speed on the Mastodon context here, but some information, if it helps:

This implementation is (AFAIK) as correct as it can be in terms of translating libpq sslmodes to pg configuration. But although it’s documented, there is one sslmode hazard in libpq we could probably improve on.

The difference between verify-ca and verify-full depends on the policy of the root CA. If a public CA is used, verify-ca allows connections to a server that somebody else may have registered with the CA. In this case, verify-full should always be used. If a local CA is used, or even a self-signed certificate, using verify-ca often provides enough protection.
I’ve already approved this PR, but since it’s a breaking change it isn’t going to be a default until pg 9. There are a lot of changes that would ideally go in pg 9, and I don’t expect it to come out soon. Maybe a new major version of pg-connection-string could be released sooner, if you’d be interested in adding a dependency on a newer version of pg-connection-string and using it manually to pass pg a configuration object instead of a connection string.
As @ThisIsMissEm mentioned in the linked pull request, pg doesn’t support sslmode=prefer (and it doesn’t support allow either). I think prefer is a bad idea (because it adds complexity and provides a false sense of security), assume/hope it’s only the libpq default for historical reasons, and don’t expect pg to support it in the future.

ThisIsMissEm · 2024-11-13T01:22:12Z

@charmander sslmode=prefer is explicitly what our users have asked for because we support it on the ruby side of the project which uses the native pg implementation

charmander · 2024-11-13T01:26:19Z

packages/pg-connection-string/index.js

+      break
+    }
+    case 'verify-ca': {
+      config.ssl.checkServerIdentity = function () {}


On second thought, even though libpq has and has documented this same problem, maybe we should enforce that config.ssl.ca is truthy here.

Suggested change

config.ssl.checkServerIdentity = function () {}

if (!config.ssl.ca) {

throw new Error('sslmode=verify-ca requires specifying a CA with sslrootcert')

}

config.ssl.checkServerIdentity = function () {}

The main issue with this would be for anyone who wants to add a ca key to the returned config.ssl after parsing. I don’t think a warning is the way to go (too easy for those who need it to miss or ignore, and annoying for those who don’t need it to suppress). Would adding a whole options parameter to support this (parse(str, {deferSslCa: true}) – probably a better name option) be too much? (Related issue, merging configuration with a connection string is hard in general: #3327)

Alternatively, the check could go in pg, e.g. by pg-connection-string exporting const NO_VERIFY = () => {} and pg throwing if config.ssl.checkServerIdentity === NO_VERIFY && !config.ssl.ca.

added the check in 74a314e

Is it really needed to enforce config.ssl.ca for this sslmode?

If config.ssl.ca is not specified, the TLS client will use the CA certificates already available via tls.getCACertificates('default'), which can include certificates included in the bundled certificate store, the system's trusted store and any extra certificate configured via the NODE_EXTRA_CA_CERTS env variable. Docs

In our implementation, we use the latter option (NODE_EXTRA_CA_CERTS) to configure the CA certificates from our Postgres endpoint and it works fine.

TIL

I will unwind this change. It will match the behavior in lib/pq@585ef5d#diff-4f427d2b022907c552328e63f137561f6de92396d7a6e8f6c2ea1bcf0db52654R836-R838

@pmalouin This is exactly why I thought a check might be useful :) Anyone can get a valid certificate from a system-trusted CA, simply by requesting one for a domain that they actually control. Without config.ssl.ca enforced, such a certificate can then be used to MITM any sslmode=verify-ca connection, providing essentially the same security level as sslmode=require.

This behavior seems consistent, well documented and understood by those who want libpq compatibility. If someone has a MITM concern, then they should use sslmode=verify-full or specify a more narrow sslrootcert.

Also, I am not sure the proposed check will have the intended results. If a user encounters this error, they will almost certainly add &sslrootcert=/etc/ssl/certs/ca-certificates.crt to the connection string and move on. In fact, I originally thought specifying the system CA (or a custom CA) was the intention of the check because I did not realize that the TLS client will use the CA certificates already available.

All that being said, I did see the following chart in the JDBC docs which outlines the risks in a way that is easy to understand. I think we should incorporate a similar chart into the README so people are aware of the MITM risks.

I mentioned that it was documented by libpq in my original comment, but I think @pmalouin’s comment demonstrates that the implications are still not obvious to people. A more descriptive error could be a good idea. I don’t think any amount of documentation is sufficient to replace the check.

charmander · 2024-11-13T02:37:09Z

@charmander sslmode=prefer is explicitly what our users have asked for because we support it on the ruby side of the project which uses the native pg implementation

@ThisIsMissEm In the end it’s up to @brianc, but I’m against supporting sslmode=prefer in pg – it’s convenient and unsafe, which isn’t a good combo.

Looking back at more of the context and questions, e.g.

I'm wanting an expert opinion on the changes in the upstream PR before I make any further changes here, since I'm by no means an expert on TLS/SSL, and I'm cautious about introducing accidental security vulnerabilities here.

sslmode=require is implemented in this PR without adding any vulnerabilities relative to libpq, but libpq’s sslmode=require is inherently vulnerable to MITM even though it might sound like “require normal SSL” by the name – it doesn’t check certificates at all, so it only provides opportunistic encryption. Most people should be using either sslmode=verify-ca (with sslrootcert) or sslmode=verify-full. verify-full is the one that gives the basic guarantees of TLS that are defaults in places like Node’s node:tls module (+ most other platforms) and web browsers.

(Re: managed database providers: It looks like DigitalOcean, the managed database provider in mastodon/mastodon#11445, is giving bad advice by encouraging people to connect with sslmode=require over the internet, and only mentioning specifying sslrootcert as an afterthought, plus a quick link to libpq documentation. Heroku is even worse: require over the internet, require from generic code, no mention of the lack of guarantees anywhere. Google Cloud SQL is fully safe, but it does its own thing and passes pre-connected streams to pg. AWS RDS documents a secure way to do it, as well as what look like some less secure ways.)

hjr3 · 2024-11-15T11:29:32Z

@charmander

I’ve already approved this PR, but since it’s a breaking change it isn’t going to be a default until pg 9.

I suggested a path forward here: #2709 (comment)

It is not clear to me why we would need to wait until pg 9 to release pg-connection-string v3.0

charmander · 2024-11-15T18:51:50Z

@hjr3 As I mentioned in the same comment:

Maybe a new major version of pg-connection-string could be released sooner, if you’d be interested in adding a dependency on a newer version of pg-connection-string and using it manually to pass pg a configuration object instead of a connection string.

i.e. we don’t need to wait, but the new major version of pg-connection-string won’t be the default dependency in pg (that it uses to parse connectionString) until pg 9, so people will still need to make application-side changes.

hjr3 · 2024-11-15T22:19:13Z

@hjr3 As I mentioned in the same comment:

Maybe a new major version of pg-connection-string could be released sooner, if you’d be interested in adding a dependency on a newer version of pg-connection-string and using it manually to pass pg a configuration object instead of a connection string.

i.e. we don’t need to wait, but the new major version of pg-connection-string won’t be the default dependency in pg (that it uses to parse connectionString) until pg 9, so people will still need to make application-side changes.

I somehow missed that. My apologies. I understand and agree with what you are saying.

I do think it is fairly common for teams to use newer versions of pg-connection-string with pg.

ThisIsMissEm · 2025-04-08T15:01:19Z

In mastodon, we already directly pull in pg-connection-string, specifically to try to better handle connection options from server operators. Would be fully in favor of a release that includes this and #3128. Would sponsorship or something help move this along? (I might be able to get some support for that)

hjr3 · 2025-04-08T16:29:25Z

I want to continue working on pg-connection-string and I do not need sponsorship. The goal of iceddev/pg-connection-string#37 was to make it easier to keep packages in sync with each other. Unfortunately, I lost my merge permissions when the transition was made.

If @brianc can give me merge permissions, I can move the package forward. If there are concerns with my ability to merge other packages, I started #3129 which should allow us to enforce the proper permissions.

ThisIsMissEm · 2025-04-08T18:32:34Z

@hjr3 that was more directed at @brianc

brianc · 2025-04-08T18:38:24Z

hey sorry I had the flu last week...literally never been that sick before in my life. still only about 80% back to health. Yeah @hjr3 I've "known" you for a long time on github, and trust you. happy to give you merge permissions! You might need to ping me if you need a critical release done to npm at any time, but even though sometimes I go awol for a week or two I am always watching the repo...just hard to do computer stuff when you think you might actually die from the flu! Instead of merging this myself I'm going to give you permissions @hjr3 so we can make sure they work. :)

brianc · 2025-04-08T18:40:16Z

@hjr3 sent you a collaborator invite.

hjr3 · 2025-04-09T00:22:25Z

I originally wanted to merge this in as-is and publish a 3.0.0. I now wonder if we should add a flag (.e.g. sslcompat=libpq) to enable this behavior in 2.x.x so users have an upgrade path. We can then make this the default behavior in 3.0.0.

ThisIsMissEm · 2025-04-09T00:40:48Z

I originally wanted to merge this in as-is and publish a 3.0.0. I now wonder if we should add a flag (.e.g. sslcompat=libpq) to enable this behavior in 2.x.x so users have an upgrade path. We can then make this the default behavior in 3.0.0.

I'd probably say just release 3.0.0 with this as default and thoroughly document the breaking changes

Change request addressed

Co-authored-by: Charmander <[email protected]>

charmander

same as my previous review

charmander approved these changes Feb 24, 2022

View reviewed changes

packages/pg-connection-string/README.md Outdated Show resolved Hide resolved

packages/pg-connection-string/test/parse.js Show resolved Hide resolved

packages/pg-connection-string/test/parse.js Show resolved Hide resolved

charmander added this to the [email protected] milestone Feb 24, 2022

ceefour mentioned this pull request Dec 28, 2022

no way to set rejectUnauthorized from pg-connection-string #2281

Open

zeeZ mentioned this pull request Oct 14, 2023

Add support for DB_SSLMODE=require in streaming API mastodon/mastodon#25483

Draft

scrpr mentioned this pull request Jun 27, 2024

[Bug] 在服务端数据库的方案中，文档需要进一步说明避免用户落到坑里 lobehub/lobe-chat#3000

Closed

wasd171 mentioned this pull request Aug 28, 2024

Align sslMode with current libpq-connect options gajus/slonik#641

Open

charmander previously requested changes Nov 13, 2024

View reviewed changes

hjr3 mentioned this pull request Nov 15, 2024

feat(pg-connection-string): ClientConfig helper functions #3128

Merged

ThisIsMissEm mentioned this pull request Mar 28, 2025

Streaming: improve handling of SSLMODE and cert/key/ca files mastodon/mastodon#31667

Open

hjr3 self-requested a review as a code owner April 9, 2025 01:20

charmander self-requested a review April 9, 2025 02:10

charmander removed their request for review April 9, 2025 02:11

pmalouin and others added 3 commits April 9, 2025 06:17

fix(pg-connection-string): get closer to libpq semantics for sslmode

1bc03de

Apply suggestions from code review

c3aebe7

Co-authored-by: Charmander <[email protected]>

introduce sslcompat=libpq flag to support libpq sslmode semantics

8d56f86

hjr3 force-pushed the sslmode-fixes branch 2 times, most recently from f58c6ff to 0d4289d Compare April 9, 2025 21:46

hjr3 added 2 commits April 9, 2025 17:51

document behavior of verify-ca for libpq compat

1164593

remove sslmode=no-verify from libpq compat

f16737e

hjr3 force-pushed the sslmode-fixes branch from 635870a to f16737e Compare April 9, 2025 21:51

charmander requested changes Apr 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(pg-connection-string): get closer to libpq semantics for `sslmode` #2709

fix(pg-connection-string): get closer to libpq semantics for `sslmode` #2709

pmalouin commented Feb 21, 2022

cyx commented Mar 24, 2022

ceefour commented Dec 28, 2022

ceefour commented Mar 16, 2023

ThisIsMissEm commented Jan 22, 2024

charmander commented Jan 23, 2024

ThisIsMissEm commented Jan 23, 2024

charmander commented Jan 23, 2024

hjr3 commented Jan 23, 2024 •

edited

Loading

ThisIsMissEm commented Jan 23, 2024

ThisIsMissEm commented Aug 10, 2024

Trott commented Nov 12, 2024

charmander commented Nov 13, 2024

ThisIsMissEm commented Nov 13, 2024

charmander Nov 13, 2024

charmander Nov 13, 2024

hjr3 Apr 9, 2025

pmalouin Apr 9, 2025

hjr3 Apr 9, 2025

charmander Apr 9, 2025 •

edited

Loading

hjr3 Apr 9, 2025

charmander Apr 9, 2025

charmander commented Nov 13, 2024 •

edited

Loading

hjr3 commented Nov 15, 2024

charmander commented Nov 15, 2024 •

edited

Loading

hjr3 commented Nov 15, 2024

ThisIsMissEm commented Apr 8, 2025

hjr3 commented Apr 8, 2025

ThisIsMissEm commented Apr 8, 2025

brianc commented Apr 8, 2025

brianc commented Apr 8, 2025

hjr3 commented Apr 9, 2025

ThisIsMissEm commented Apr 9, 2025

charmander left a comment

fix(pg-connection-string): get closer to libpq semantics for sslmode #2709

Are you sure you want to change the base?

fix(pg-connection-string): get closer to libpq semantics for sslmode #2709

Conversation

pmalouin commented Feb 21, 2022

Summary

Detailed sslmode behavior

sslmode=verify-full

sslmode=verify-ca (changed)

sslmode=require (changed)

sslmode=no-verify

sslmode=prefer (changed)

sslmode=allow

sslmode=disable

Prior discussions about sslmode

Other languages/libraries and their support for sslmode

cyx commented Mar 24, 2022

ceefour commented Dec 28, 2022

ceefour commented Mar 16, 2023

ThisIsMissEm commented Jan 22, 2024

charmander commented Jan 23, 2024

ThisIsMissEm commented Jan 23, 2024

charmander commented Jan 23, 2024

hjr3 commented Jan 23, 2024 • edited Loading

ThisIsMissEm commented Jan 23, 2024

ThisIsMissEm commented Aug 10, 2024

Trott commented Nov 12, 2024

charmander commented Nov 13, 2024

ThisIsMissEm commented Nov 13, 2024

charmander Nov 13, 2024

Choose a reason for hiding this comment

charmander Nov 13, 2024

Choose a reason for hiding this comment

hjr3 Apr 9, 2025

Choose a reason for hiding this comment

pmalouin Apr 9, 2025

Choose a reason for hiding this comment

hjr3 Apr 9, 2025

Choose a reason for hiding this comment

charmander Apr 9, 2025 • edited Loading

Choose a reason for hiding this comment

hjr3 Apr 9, 2025

Choose a reason for hiding this comment

charmander Apr 9, 2025

Choose a reason for hiding this comment

charmander commented Nov 13, 2024 • edited Loading

hjr3 commented Nov 15, 2024

charmander commented Nov 15, 2024 • edited Loading

hjr3 commented Nov 15, 2024

ThisIsMissEm commented Apr 8, 2025

hjr3 commented Apr 8, 2025

ThisIsMissEm commented Apr 8, 2025

brianc commented Apr 8, 2025

brianc commented Apr 8, 2025

hjr3 commented Apr 9, 2025

ThisIsMissEm commented Apr 9, 2025

charmander left a comment

Choose a reason for hiding this comment

fix(pg-connection-string): get closer to libpq semantics for `sslmode` #2709

fix(pg-connection-string): get closer to libpq semantics for `sslmode` #2709

Detailed `sslmode` behavior

`sslmode=verify-full`

`sslmode=verify-ca` (changed)

`sslmode=require` (changed)

`sslmode=no-verify`

`sslmode=prefer` (changed)

`sslmode=allow`

`sslmode=disable`

Prior discussions about `sslmode`

Other languages/libraries and their support for `sslmode`

hjr3 commented Jan 23, 2024 •

edited

Loading

charmander Apr 9, 2025 •

edited

Loading

charmander commented Nov 13, 2024 •

edited

Loading

charmander commented Nov 15, 2024 •

edited

Loading